Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Support resolving module.register dependencies #429

Merged
merged 16 commits into from
Sep 3, 2024
Merged

fix: Support resolving module.register dependencies #429

merged 16 commits into from
Sep 3, 2024

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Jun 25, 2024 •
edited
Loading

@timfish timfish requested review from ijjk and styfle as code owners June 25, 2024 21:32
styfle
styfle reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Can you change the integration test into a unit test instead?

That way we can see see the input and output to make sure these remain the same instead of relying on implementation details of a 3rd party package.

styfle
styfle reviewed Jun 26, 2024
View reviewed changes
src/analyze.ts Outdated Show resolved Hide resolved
@timfish timfish requested a review from styfle June 27, 2024 17:36
styfle
styfle reviewed Jun 28, 2024
View reviewed changes
src/analyze.ts Outdated Show resolved Hide resolved
@timfish
Copy link
Contributor Author

timfish commented Jul 3, 2024

Looks like I need to pull my Windows laptop out and work out why those tests are failing!

timfish
timfish commented Jul 3, 2024
View reviewed changes
src/analyze.ts Show resolved Hide resolved
@timfish
Copy link
Contributor Author

timfish commented Jul 3, 2024

So the failing unit test (zeromq-node-gyp) passes in isolation on my Windows machine but fails when all the tests are run.

One thing I don't understand is how it's passing at all, even on other platforms.

The expected output is:

nft/test/unit/zeromq-node-gyp/output.js

Lines 1 to 11 in 31ab417

[
"node_modules/@aminya/node-gyp-build/index.js",
"node_modules/@aminya/node-gyp-build/package.json",
"node_modules/zeromq/lib/draft.js",
"node_modules/zeromq/lib/index.js",
"node_modules/zeromq/lib/native.js",
"node_modules/zeromq/lib/util.js",
"node_modules/zeromq/package.json",
"package.json",
"test/unit/zeromq-node-gyp/input.js"
]

But it's failing with an extra file included:

        "node_modules\\@aminya\\node-gyp-build\\index.js",
    +   "node_modules\\@aminya\\node-gyp-build\\node-gyp-build.js",
        "node_modules\\@aminya\\node-gyp-build\\package.json",
        "node_modules\\zeromq\\lib\\draft.js",
        "node_modules\\zeromq\\lib\\index.js",
        "node_modules\\zeromq\\lib\\native.js",
        "node_modules\\zeromq\\lib\\util.js",

And the reason it's being included:

      'node_modules\\@aminya\\node-gyp-build\\node-gyp-build.js' => {
        type: [ 'dependency' ],
        ignored: false,
        parents: Set(1) { 'node_modules\\@aminya\\node-gyp-build\\index.js' }
      }

And then when you look at the actual source of the parent, it's obvious why this is being included:

} else { // else use the runtime version here
  module.exports = require('./node-gyp-build.js')
}

So how is this passing on every other platform but failing on Windows when the full test suite is run?

@styfle styfle requested a review from a team as a code owner July 3, 2024 21:35
@styfle
Copy link
Member

styfle commented Jul 3, 2024

This might fix it: #432

I merged it in to see if tests pass now 🤞

@timfish
Copy link
Contributor Author

timfish commented Jul 3, 2024

Unfortunatly not 😭

I'm more confused as to how this test passes on any platform. The additional dependency in the failing case looks like it should be included!

@styfle
Copy link
Member

styfle commented Jul 3, 2024 •
edited
Loading

Looks like the only failing test is the new one now. Its missing hook.mjs, hook2.mjs and hook3.mjs.

 ● should correctly trace module-register from cwd

    expect(received).toEqual(expected) // deep equality

    - Expected  - 3
    + Received  + 0

      Array [
        "package.json",
    -   "test\\unit\\module-register\\hook.mjs",
    -   "test\\unit\\module-register\\hook2.mjs",
    -   "test\\unit\\module-register\\hook3.mjs",
        "test\\unit\\module-register\\input-esm.mjs",
        "test\\unit\\module-register\\input.js",
        "test\\unit\\module-register\\node_modules\\test-pkg\\index.mjs",
        "test\\unit\\module-register\\node_modules\\test-pkg\\package.json",
      ]

https://github.com/vercel/nft/actions/runs/9785332243/job/27018410588?pr=429#step:9:1063

@timfish

This comment was marked as outdated.

Sign in to view
@timfish
Copy link
Contributor Author

timfish commented Jul 4, 2024 •
edited
Loading

So I reverted the path.sep change and the tests now all pass on Windows.

Looking at the resulting deps and imports and on Windows for ALL the tests, these all contain forwards slashes /.

@timfish timfish mentioned this pull request Jul 10, 2024
Open
@s1gr1d
Copy link

s1gr1d commented Jul 22, 2024

@styfle can this be merged?

EndangeredMassa
EndangeredMassa requested changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@EndangeredMassa EndangeredMassa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but there's one case we should add.

Thanks!

src/analyze.ts Show resolved Hide resolved
@AbhiPrasad AbhiPrasad mentioned this pull request Aug 8, 2024
Open
styfle
styfle approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@EndangeredMassa Want to give it another look?

@s1gr1d s1gr1d mentioned this pull request Sep 2, 2024
Closed
@EndangeredMassa EndangeredMassa added the automerge Automatically merge PR once checks pass label Sep 3, 2024
@kodiakhq kodiakhq bot merged commit 37b3c16 into vercel:main Sep 3, 2024
11 checks passed
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 3, 2024

🎉 This PR is included in version 0.27.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@s1gr1d s1gr1d mentioned this pull request Sep 4, 2024
Closed
@leonardoadame leonardoadame mentioned this pull request Sep 24, 2024
Open
@Thompson1985 Thompson1985 mentioned this pull request Oct 7, 2024
Open
@styfle styfle mentioned this pull request Oct 26, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onsclom onsclom onsclom approved these changes

@styfle styfle styfle approved these changes

@EndangeredMassa EndangeredMassa EndangeredMassa approved these changes

@ijjk ijjk Awaiting requested review from ijjk ijjk is a code owner

Assignees
No one assigned
Labels
automerge Automatically merge PR once checks pass released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Does not include registered ESM loader hooks
5 participants
@timfish @styfle @s1gr1d @EndangeredMassa @onsclom